Cell diagnostics#980
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
@vezwork @juliasilge I'm not sure who would be the best person to review this. It is big (although ~half is tests and test infra) so I'm happy to walk either/both of you through it in a call. |
If a user opens One eventual (but rapidly coming) goal of ark is that diagnostics on But if you were to open, say, So if you create That may not be exactly how ark works today, but we are rapidly iterating our way towards this kind of design |
| let timeout: NodeJS.Timeout; | ||
| const timeoutPromise = new Promise<undefined>((resolve) => { | ||
| timeout = setTimeout(() => resolve(undefined), ms); | ||
| }); | ||
| return Promise.race([ | ||
| promise.finally(() => clearTimeout(timeout)), |
There was a problem hiding this comment.
nit: can just race against wait(ms) instead? I don't think clearing the timeout is necessary.
| trigger?: string[]; | ||
| /** | ||
| * Lines of code to inject at the top of the virtual document. | ||
| * Used to disable diagnostics for virtual documents that were |
There was a problem hiding this comment.
| * Used to disable diagnostics for virtual documents that were | |
| * Used to add lines that hint to language LSPs to disable diagnostics for virtual documents that were |
| emptyLine?: string; | ||
| /** | ||
| * Lines of code to inject at the top of the virtual document. | ||
| * Used to disable diagnostics for virtual documents that were |
There was a problem hiding this comment.
| * Used to disable diagnostics for virtual documents that were | |
| * Used to add lines that hint to language LSPs to disable diagnostics for virtual documents that were |
| import { VirtualDoc, VirtualDocUri } from "./vdoc"; | ||
|
|
||
| interface VirtualDocTempFileOptions { | ||
| /** Fire a hover request to prime the language server before returning. */ |
There was a problem hiding this comment.
| /** Fire a hover request to prime the language server before returning. */ | |
| /** Fire a "dummy" hover request to cause the language server to start */ |
| const testFiles = glob.sync("src/test/{*.ts,fixtures/*.ts,utils/*.ts}"); | ||
|
|
||
| const testBuildOptions = { | ||
| entryPoints: testFiles, | ||
| outdir: 'test-out', | ||
| external: ['vscode', 'mocha', 'glob'], | ||
| sourcemap: true, | ||
| dev, | ||
| }; | ||
|
|
||
| const defaultBuildOptions = { | ||
| entryPoints: ['./src/main.ts'], | ||
| outfile: './out/main.js', | ||
| external: ['vscode'], | ||
| minify: !dev, | ||
| dev | ||
| }; | ||
|
|
||
| if (test) { | ||
| runBuild(testBuildOptions); | ||
| } else if (dev) { | ||
| runBuild(defaultBuildOptions); | ||
| runBuild(testBuildOptions); | ||
| } else { | ||
| runBuild(defaultBuildOptions); | ||
| } |
There was a problem hiding this comment.
Didn't we merge these changes into main already? Not sure why these are showing up as changes in this PR.
|
Tested locally and I am getting red and blue underlines in R and Python cells in VSCode, and red and blue underlines in Python cells in Positron. |
this wasn't doing anything since the diagnostics for vdocs are handled by external language clients - not ours
Replace the deferred-promise/withVirtualDocUri architecture with independent DiagnosticSession objects — one per language per document. Each session manages its own vdoc lifecycle and timeout, so a non-responsive language server for one language doesn't block or interfere with another language's diagnostics. - Delete resource-map.ts (no longer needed) - Add optional timeoutMs constructor param (defaults to 10s) - Sessions merge diagnostics across languages when publishing - Add diagnostics-multilang.qmd test fixture - Update test language client to handle R documents - Add multi-language test verifying independent per-language diagnostics
…f separate function
…extDiagnostics/nextVdocDisposal Subscribe-then-act pattern is now linear instead of nested via callbacks.
Replace the flat array with a two-level map plus a vdoc URI reverse index. All lookups are now O(1) and the two-level keying (document × language) is explicit in the type system.
The test language server is spawned as a child process by the LanguageClient, so it can't be part of the esbuild bundle. Converting it to plain JS means it never goes through the bundler and can be referenced directly from source.
vezwork
force-pushed
the
feat/cell-diagnostics-wl
branch
from
53a28a6 to
eb288f5
Compare
|
rebased on main, updated changelog, and force pushed (excuse me, didn't mean to add myself as author to all these commits :|) |
juliasilge
left a comment
juliasilge
left a comment
There was a problem hiding this comment.
This is working remarkably well as I put it through its paces:
I am really impressed. 😅
I will say that in the long run for Positron, I am still going to advocate that we take a different approach. I think an in-memory representation of .qmd that is very, very notebook-like will be a better option, both for LSP features as well as AI features. I am actually so eager for us to do that work as outlined in posit-dev/positron#12837 that I would advocate we do that instead of making the Ark LSP work with this new approach in this PR. That new document model approach also sets us up for the LSP work that @DavisVaughan mentions that would be truly difficult/impossible with vdoc files.
That being said, it is incredible to me that this works and I think we should merge it, pending some of the minor feedback on the PR. This would be a really huge win for VS Code (i.e. not Positron) users.
| for (const diagnostic of rawDiagnostics) { | ||
| const block = languageBlockAtPosition(session.languageBlocks, diagnostic.range.start); | ||
| if (block !== undefined) { | ||
| mapped.push(new Diagnostic(diagnostic.range, diagnostic.message, diagnostic.severity)); |
There was a problem hiding this comment.
Is it not possible map the whole Diagnostic here, with the other components that may be offered by the LSP? You can read more here:
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#diagnostic
| await this.createSessionsForDocument(document); | ||
| } | ||
|
|
||
| private async activateSession( |
There was a problem hiding this comment.
IIUC this awaits the vdoc being created and we could get a race between real document changes and a stale vdoc. Can we keep old async work from mutating manager state after a newer session has taken over? Something in here where we check if it's still the current session?
5 participants
Addresses #208, joint work with @vezwork, building on #957. Here's how it works as described in the original PR:
We improved on that by creating vdocs in the temp dir when possible (pyrefly and ark) to keep it out of the user's workspace, and by deleting the vdoc once diagnostics are received.
The vscode-R extension is an exception and requires vdocs in the workspace, so we put it under
.quarto.Here's the full sequence diagram:
sequenceDiagram actor User participant VSC as VS Code participant Mgr as EmbeddedDiagnosticsManager<br/>(Quarto extension) participant LC as LanguageClient<br/>(vscode-languageclient) participant LS as Language Server<br/>(e.g. Pyrefly, Ark) User->>Mgr: User action initiates diagnostic request<br/>e.g. document open or edit (after debounce) activate Mgr Mgr->>Mgr: Parse and group cells by language loop For each language found in the document create participant VDoc as Virtual Document<br/>(temp file on disk) Mgr->>VDoc: Create temp file with code<br/>per language Mgr->>VSC: openTextDocument(vdoc URI) deactivate Mgr activate VSC VSC->>LC: onDidOpenTextDocument(vdoc) deactivate VSC activate LC LC->>LS: textDocument/didOpen (vdoc) deactivate LC activate LS LS-->>LS: Analyze document LS->>LC: textDocument/publishDiagnostics deactivate LS activate LC LC->>VSC: Set diagnostics for vdoc URI deactivate LC activate VSC VSC-->>User: ⚠️ Vdoc entry briefly visible<br/>in Problems pane (known issue) VSC->>Mgr: onDidChangeDiagnostics(vdoc URI) deactivate VSC activate Mgr par Publish .qmd cell diagnostics Mgr->>Mgr: Filter diagnostics to code cell ranges Mgr->>VSC: Set diagnostics for .qmd URI activate VSC VSC->>User: Squiggly underlines in editor +<br/>entries in Problems pane deactivate VSC and Cleanup Mgr->>VSC: Set vdoc language to plaintext activate VSC destroy VDoc Mgr-xVDoc: Delete temp file deactivate Mgr VSC->>LC: onDidCloseTextDocument(vdoc) deactivate VSC activate LC LC->>LS: textDocument/didClose deactivate LC activate LS LS->>LC: textDocument/publishDiagnostics (empty) deactivate LS activate LC LC->>VSC: Clear diagnostics for vdoc URI deactivate LC activate VSC VSC-->>User: Vdoc entry removed from Problems pane deactivate VSC end endClearing vdoc diagnostics
This also includes a fix to get vdoc diagnostics to actually clear. The problem is that VS Code constrains extensions' ability to close text documents and fire
onDidCloseTextDocument, which language clients forward to language servers astextDocument/didClosenotifications, and which they generally respond to by clearing diagnostics.One option was to use
WorkspaceEditto delete the file which would fire the event, but it doesn't let us skip the trash.The hack in this PR is to change the vdoc's language (set it to
plaintext), which sendstextDocument/didCloseand clears vdoc diagnostics.Known issues
Tests
There is an extensive e2e test suite included. It uses a fake language server, but otherwise exercises the full production code paths, and covers a bunch of edge cases that I ran into while working on this.
There's a bunch of test infra added that might be useful for testing the other language features in future.